Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i18n: Add full support #399

Closed
wants to merge 17 commits into from
Closed

i18n: Add full support #399

wants to merge 17 commits into from

Conversation

jpmckinney
Copy link
Contributor

@jpmckinney jpmckinney commented Apr 22, 2021

resolves #257

  • Translate PO files
  • Generate MO files
  • Test with another site

@jpmckinney
Copy link
Contributor Author

This should work now. I'll test on my docs site.

@jpmckinney
Copy link
Contributor Author

Okay, this is ready for review. It works!

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! A few quick comments in there. Also a couple points that didn’t make sense for in line comments

  • is there a natural way to test for this infrastructure? Do most packages include translation testing in their test suite? Ideally without complicating things much?
  • We should add in a bit more explanation about how the translation infra works - I added a few suggestions in the review. In my experience people don’t know anything about Sphinx translation so this will help get oriented

Also feel like we should get sign off from @jorisvandenbossche to make sure he’s ok with this new infra setup

docs/user_guide/i18n.rst Outdated Show resolved Hide resolved
docs/user_guide/i18n.rst Outdated Show resolved Hide resolved
pydata_sphinx_theme/icon-links.html Show resolved Hide resolved
@@ -529,7 +532,7 @@ def setup(app):

# Update templates for sidebar
pkgdir = os.path.abspath(os.path.dirname(__file__))
path_templates = os.path.join(pkgdir, "_templates")
path_templates = os.path.join(pkgdir, "templates")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flag for @jorisvandenbossche, any reason we wouldn’t want to use templates wo the underscore? I don’t think this would affect anything downstream

Copy link
Contributor Author

@jpmckinney jpmckinney Apr 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't affect theme users. When theme users override a template, it's based on the file's basename, not on its path within a particular theme, to my knowledge.

docs/contributing.rst Show resolved Hide resolved
docs/contributing.rst Outdated Show resolved Hide resolved
docs/user_guide/i18n.rst Show resolved Hide resolved

This theme contains translatable strings. Most strings will automatically be translated if the language is `supported <https://github.com/pydata/pydata-sphinx-theme/tree/master/pydata_sphinx_theme/locale>`__. To add another language, see :ref:`translating-the-theme`.

The theme's :doc:`options<configuring>` also contain translatable strings, which require a different process. The following instructions assume that you store your translations in a ``locale`` directory under your documentation directory, and that you want to use ``theme`` as the name the message catalog for these strings.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should link to a useful introduction for Sphinx translation, in my experience most users don’t know how this works so I think this would helo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that any are especially helpful. See e.g. https://www.sphinx-doc.org/en/master/usage/advanced/intl.html

jpmckinney and others added 2 commits April 24, 2021 16:30
@jpmckinney
Copy link
Contributor Author

I've updated contributing.rst and accepted the suggestions. Not clear what to change in i18n.rst.

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for those edits, this is looking really good. A few more relatively small comments.

docs/contributing.rst Outdated Show resolved Hide resolved
docs/contributing.rst Outdated Show resolved Hide resolved
docs/user_guide/i18n.rst Outdated Show resolved Hide resolved
@jorisvandenbossche
Copy link
Member

Quick comment: fully on board with improving the theme in this area, assuming we follow the sphinx standard practices (and @jpmckinney thanks a lot for working on this!)
Will try to take a more detailed look in one of the coming days

@jpmckinney
Copy link
Contributor Author

jpmckinney commented Apr 27, 2021

is there a natural way to test for this infrastructure? Do most packages include translation testing in their test suite? Ideally without complicating things much?

Different projects do different things.

  • If you want every PR to always be 100% translated, you can add a test using pocount --incomplete *.po to ensure there are no untranslated strings.
  • If you want to ensure the POT file is up-to-date, you can have the tests run python setup.py extract_messages --omit-header --no-location and check for changes. Without --omit-header, the POT-Creation-Date will be updated by the test, causing a failure. Without --no-location, whenever lines are added to templates before translatable strings, the #: source code reference lines in the POT file will go out of date, causing a failure.
  • If you want to ensure the PO files are up-to-date, you can have the tests run python setup.py update_catalog --omit-header and check for changes.

Alternately, you can decide to do the translations steps before a release only.

@choldgraf
Copy link
Collaborator

Hey all - I think that this PR is an important improvement in the theme, and we don't want it to go unresolved indefinitely. Since we've already got a lot of iteration here, and since @jpmckinney has put a lot of time into this already. I suggest we go with the following steps:

  1. @jpmckinney rebases and resolves merge conflicts.
  2. We leave this PR open for 48 hours to see if there are any objections to merging it.
  3. We merge the PR and spot-check other i18n improvements in other issues.

@jpmckinney would you be willing to do no. 1? Sorry that this one has gone stale!

@jpmckinney
Copy link
Contributor Author

Restarted at #1192.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Translations for page elements
3 participants